Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle collections in root of site (#18) #19

Merged
merged 15 commits into from
Apr 11, 2024

Conversation

techfg
Copy link
Contributor

@techfg techfg commented Apr 8, 2024

Resolves #18

I based this PR off of PR #17 as it made solutioning for the issues covered in this one more straightforward to resolve than the prior code base would have. With that said, the solution I came up with does introduce a new option collectionPathMode.

Upon reviewing the potential approaches for addressing the issues, I just could not see a way to reliably and with certainty determine the collection path from the information we have available within the plugin. The prior code and even PR #17 and this PR still assume that a content collection is a subdirectory of the content path - normal behavior in Astro - but not always the case as #18 describes.

Given that, the only way to know for certain if the content collection and content path are the same is for the consumer of the library to tell us 😦

The default behavior (ContentPathMode = 'subdirectory') still applies and works identical to the way it did prior to this PR where we assume the content collections are immediate child directories of the content path. This PR simply allows for them to be the same on an opt-in basis. The same notes/warnings apply from those mentioned in Additional Information` in the OP of #18.

@techfg techfg changed the title fix for #18 fix: handle collections in root of site (#18) Apr 9, 2024
@vernak2539
Copy link
Owner

After the merge of #17 this PR now needs some merge conflicts resolved 😅

@techfg
Copy link
Contributor Author

techfg commented Apr 11, 2024

No worries :) I'll merge but we might not need this - see #27 for some thoughts and lemme know what you think.

@techfg
Copy link
Contributor Author

techfg commented Apr 11, 2024

Just pushed a merge but its not correct as I double up some tests - fixing now and will push again, sorry!

@techfg
Copy link
Contributor Author

techfg commented Apr 11, 2024

@vernak2539 - Ok, merge fixed, all set for you. FYI, similar to prior commits, I did not format the files to minimize diff alignment issues so they could likely use one :)

@techfg
Copy link
Contributor Author

techfg commented Apr 11, 2024

No worries :) I'll merge but we might not need this - see #27 for some thoughts and lemme know what you think.

Also, lemme know your thoughts on this and to a lesser but related degree your thoughts on #26, as depending on what they are, we might not need this PR.

@vernak2539
Copy link
Owner

Interesting. I'll need a bit more time to parse and think about #26 and #27.

But, that doesn't mean we can't merge this one first and go from there. Will review now

update ternary check to be explicit about check, instead of just falsy check
@vernak2539 vernak2539 merged commit fc33147 into vernak2539:main Apr 11, 2024
2 checks passed
@techfg
Copy link
Contributor Author

techfg commented Apr 11, 2024

Yeah, I'm still trying to think through all the scenarios across both issues lol

I think 27 has merit and might be a better path forward, more consistent with how Astro handles things internally. Still want to think it through some more.

For 26, I think we are going to run in to issues if we try to cross collection boundaries but there are benefits if the physical file structure matches the page path structure identically.

For this PR, even if we don't need collectionPathMode after deciding on 27, can always remove since we're pre-v1.

Look forward to your thoughts on 26 & 27 and lemme know if you have any questions on this PR in meantime.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: When contentPath is the same as the collectionPath, some transformed paths are incorrect
2 participants